Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-38323: Change MultiLoopChildWatcher to install handlers for all the event loops #26574

Closed
wants to merge 9 commits into from

Conversation

shreyanavigyan
Copy link
Contributor

@shreyanavigyan shreyanavigyan commented Jun 7, 2021

Fix a race condition in MultiLoopChildWatcher. This patch does the following -

  1. Creates a list that will keep track of the event loops where the handler has been installed by MultiLoopChildWatcher.
  2. In attach_loop and add_child_handler, the handler is installed for the loop using add_signal_handler and the event loop instance is also added to the list.
  3. In remove_child_handler, the remove_signal_handler is called on the loop the provided pid is attached to.
  4. In close, the remove_signal_handler is called on every loop and the loop is removed from the list.

This patch changes the behavior of MultiLoopChildWatcher. The original idea remains the same but with this patch MultiLoopChildWatcher can only be used in the main thread.

https://bugs.python.org/issue38323

@erlend-aasland
Copy link
Contributor

Please remove the [asyncio] prefix from the title.

@shreyanavigyan
Copy link
Contributor Author

Ok

@shreyanavigyan shreyanavigyan changed the title [asyncio] bpo-38323: Change MultiLoopChildWatcher to install handlers for all the event loops bpo-38323: Change MultiLoopChildWatcher to install handlers for all the event loops Jun 7, 2021
@vstinner
Copy link
Member

vstinner commented Jun 7, 2021

I'm not sure that this issue solves MultiLoopChildWatcher design issue. The MultiLoopChildWatcher doc says that it can be used even if no event loop runs in the main thread, whereas add_signal_handler() must be called from the main thread:

https://docs.python.org/dev/library/asyncio-eventloop.html#asyncio.loop.add_signal_handler

@shreyanavigyan
Copy link
Contributor Author

shreyanavigyan commented Jun 7, 2021

I'm not sure that this issue solves MultiLoopChildWatcher design issue. The MultiLoopChildWatcher doc says that it can be used even if no event loop runs in the main thread, whereas add_signal_handler() must be called from the main thread:

https://docs.python.org/dev/library/asyncio-eventloop.html#asyncio.loop.add_signal_handler

This PR changes MultiLoopChildWatcher in a way that it still doesn't require a running loop in the main thread. So the basic idea remains the same. The only change is that MultiLoopChildWatcher now can't be used from any other thread. I've tested it in the REPL and also the test ran for about 30 minutes, so I guess this patch works?

@erlend-aasland
Copy link
Contributor

Breaking backwards compatibility is not an option for 3.9. I'd say the same for 3.10, since we're past feature freeze; beta phase is all about fixing regressions (and potentially ripping out newly introduced features).

@shreyanavigyan
Copy link
Contributor Author

shreyanavigyan commented Jun 7, 2021

Breaking backwards compatibility is not an option for 3.9. I'd say the same for 3.10, since we're past feature freeze; beta phase is all about fixing regressions (and potentially ripping out newly introduced features).

For 3.9, I agree. But since this is a bug wouldn't it be better to fix it in 3.10 beta?

@erlend-aasland
Copy link
Contributor

IMO, it's better to deprecate the class in 3.10 and remove it in 3.12, and for 3.9 do nothing. But, like Victor, I'm not an asyncio user, nor do I have any knowledge of that module, so my opinion does not really matter here.

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jul 10, 2021
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 9, 2022
@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Nov 25, 2022

MultiLoopChildWatcher is deprecated now and the issue is closed so closing. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants